feat: ica submission strategy support in the cli #4980
Conversation
…to xeno/update-ism-derivation-for-ica-support
…ta building from on chain data to enable self relaying of ICA messages
…to xeno/update-ism-derivation-for-ica-support
…dule delta if the ism is an ica ism
…to xeno/ica-submission-strategy-support-in-the-cli
…if the config specifies so
…one is supplied in the strategy file when using ICAs
…s if none is supplied in the config
…to xeno/ica-submission-strategy-support-in-the-cli
…to xeno/ica-submission-strategy-support-in-the-cli
…to xeno/ica-submission-strategy-support-in-the-cli
🦋 Changeset detectedLatest commit: 6b6357c The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4980 +/- ##
=======================================
Coverage 77.53% 77.53%
=======================================
Files 103 103
Lines 2110 2110
Branches 190 190
=======================================
Hits 1636 1636
Misses 453 453
Partials 21 21
|
… to send them when provided with the appropriate strategy config
…to xeno/ica-submission-strategy-support-in-the-cli
…to xeno/ica-submission-strategy-support-in-the-cli
| .then((address) => [ | ||
| multiProvider.getChainName(domainId), | ||
| { interchainAccountRouter: bytes32ToAddress(address) }, | ||
| ]), |
There was a problem hiding this comment.
nit: avoid callback syntax and use async/await
| .discriminatedUnion('type', [ | ||
| z.object({ | ||
| type: z.literal(TxSubmitterType.JSON_RPC), | ||
| }), | ||
| z | ||
| .object({ | ||
| type: z.literal(TxSubmitterType.GNOSIS_TX_BUILDER), | ||
| }) | ||
| .merge(EV5GnosisSafeTxBuilderPropsSchema.omit({ chain: true })), | ||
| z | ||
| .object({ | ||
| type: z.literal(TxSubmitterType.GNOSIS_SAFE), | ||
| }) | ||
| .merge(EV5GnosisSafeTxSubmitterPropsSchema.omit({ chain: true })), | ||
| z | ||
| .object({ | ||
| type: z.literal(TxSubmitterType.IMPERSONATED_ACCOUNT), | ||
| }) | ||
| .merge( | ||
| EV5ImpersonatedAccountTxSubmitterPropsSchema.omit({ chain: true }), | ||
| ), | ||
| ]) | ||
| .default({ | ||
| type: TxSubmitterType.JSON_RPC, | ||
| }); |
There was a problem hiding this comment.
instead of duplicating the union here, can we just make this a "recursive" definition and have internalSubmitter be the top level submitter schema?
There was a problem hiding this comment.
otherwise this will be come stale when new submitters are added
There was a problem hiding this comment.
Oky, should we then add a limit on the nesting it can be done when creating the strategy config? I created the subtype to make it more difficult to create an invalid/overly complex config
| const EvmIcaTxSubmitterConfigSchema = EvmIcaTxSubmitterPropsSchema.required({ | ||
| originInterchainAccountRouter: true, | ||
| }); | ||
|
|
||
| /* eslint-disable-next-line @typescript-eslint/no-unused-vars */ | ||
| const EvmIcaTxSubmitterConstructorConfigSchema = | ||
| EvmIcaTxSubmitterConfigSchema.omit({ internalSubmitter: true }).required({ | ||
| owner: true, | ||
| }); | ||
|
|
||
| type EvmIcaTxSubmitterConfig = z.infer<typeof EvmIcaTxSubmitterConfigSchema>; | ||
| type EvmIcaTxSubmitterConstructorConfig = z.infer< | ||
| typeof EvmIcaTxSubmitterConstructorConfigSchema | ||
| >; | ||
|
|
||
| type SubmitterFactoryMapping< | ||
| E extends TxSubmitterType, | ||
| TConfig extends { type: E }, | ||
| TResult, | ||
| > = { | ||
| [K in E]: ( | ||
| config: Extract<TConfig, { type: K }>, | ||
| ) => Promise<TResult> | TResult; | ||
| }; |
There was a problem hiding this comment.
im a bit confused by the need for all of these
There was a problem hiding this comment.
I would expect to see a similar negative diff for removing the ICA transformer?
There was a problem hiding this comment.
shouldnt this transformer directory be removed entirely?
| import { EV5GnosisSafeTxBuilder } from './ethersV5/EV5GnosisSafeTxBuilder.js'; | ||
| import { EV5GnosisSafeTxSubmitter } from './ethersV5/EV5GnosisSafeTxSubmitter.js'; | ||
| import { EV5ImpersonatedAccountTxSubmitter } from './ethersV5/EV5ImpersonatedAccountTxSubmitter.js'; | ||
| import { EV5JsonRpcTxSubmitter } from './ethersV5/EV5JsonRpcTxSubmitter.js'; |
There was a problem hiding this comment.
I really think the ICA submitter should not need to know about these other submitters
| async function getInternalSubmitter( | ||
| chain: ChainName, | ||
| multiProvider: MultiProvider, | ||
| config: EvmIcaTxSubmitterConfig['internalSubmitter'], | ||
| ): Promise<TxSubmitterInterface<ProtocolType.Ethereum>> { | ||
| const internalSubmitterMap: SubmitterFactoryMapping< | ||
| EvmIcaTxSubmitterConfig['internalSubmitter']['type'], | ||
| EvmIcaTxSubmitterConfig['internalSubmitter'], | ||
| TxSubmitterInterface<ProtocolType.Ethereum> | ||
| > = { | ||
| [TxSubmitterType.GNOSIS_SAFE]: (config) => { | ||
| return EV5GnosisSafeTxSubmitter.create(multiProvider, { | ||
| chain, | ||
| ...config, | ||
| }); | ||
| }, | ||
| [TxSubmitterType.GNOSIS_TX_BUILDER]: (config) => { | ||
| return EV5GnosisSafeTxBuilder.create(multiProvider, { | ||
| chain, | ||
| ...config, | ||
| }); | ||
| }, | ||
| [TxSubmitterType.IMPERSONATED_ACCOUNT]: (config) => { | ||
| return new EV5ImpersonatedAccountTxSubmitter(multiProvider, { | ||
| chain, | ||
| ...config, | ||
| }); | ||
| }, | ||
| [TxSubmitterType.JSON_RPC]: (config) => { | ||
| return new EV5JsonRpcTxSubmitter(multiProvider, { | ||
| chain, | ||
| ...config, | ||
| }); | ||
| }, | ||
| }; | ||
|
|
||
| const internalSubmitterFactory = internalSubmitterMap[config.type]; | ||
| // Sanity check | ||
| if (!internalSubmitterFactory) { | ||
| throw new Error( | ||
| `Internal submitter factory not found for type: ${config.type}`, | ||
| ); | ||
| } | ||
|
|
||
| return internalSubmitterFactory(config as any); | ||
| } |
There was a problem hiding this comment.
ditto here
this doesnt seem like it should be necessary for the ICA submitter to implement
| readonly txSubmitterType: TxSubmitterType = | ||
| TxSubmitterType.INTERCHAIN_ACCOUNT; | ||
|
|
||
| private constructor( |
| const owner = | ||
| config.owner ?? (await multiProvider.getSignerAddress(config.chain)); |
There was a problem hiding this comment.
this seems a little dangerous
I would be in favor of making the origin chain and owner address required fields to avoid potential mixups like the one we had
There was a problem hiding this comment.
oky I'll make it required. I agree that it makes it less error-prone and clearer at the same time.
| export const EvmIcaTxSubmitterPropsSchema = z.object({ | ||
| chain: ZChainName, | ||
| owner: ZHash.optional(), | ||
| destinationChain: ZChainName, |
There was a problem hiding this comment.
I am actually a bit confused by the relationship between the chains/owner and internal submitter
if the internal submitter is a safe submitter then the owner address would have to match? should we remove the chain and owner from this schema entirely?
There was a problem hiding this comment.
The chain field specifies which chain the ICA transactions will be sent from, while the owner address is the owner of the ICA so yes in case the internal submitter is a safe, the addresses should match. I think I can enforce this requirement in the zod validator.
Description
This PR updates the implementation for sending ICA transactions using the CLI when running
warp applyand adds arelayflag so that ICA transactions can be self-relayed when using the JSON RPC strategy as the internal submission strategy.Drive-by changes
Related issues
Backward compatibility
Testing